Skip to content

fix(skill): block @greptileai trigger until all comments replied#656

Merged
carlos-alm merged 5 commits intomainfrom
fix/review-skill-greptile-guard
Mar 27, 2026
Merged

fix(skill): block @greptileai trigger until all comments replied#656
carlos-alm merged 5 commits intomainfrom
fix/review-skill-greptile-guard

Conversation

@carlos-alm
Copy link
Copy Markdown
Contributor

Summary

  • Adds a mandatory pre-trigger verification script to /review Step 2g that checks every Greptile inline comment has at least one reply before allowing @greptileai to be posted
  • Updates the Rules section: replaces "always re-trigger after replying" with "never trigger without replying to every comment first" — making the reply-before-trigger order an explicit blocking invariant
  • Prevents the review skill from starting new review cycles while old feedback is still unanswered

Test plan

  • Run /review on a PR with unanswered Greptile comments — verify the verification script blocks the trigger
  • Run /review on a PR where all Greptile comments have replies — verify it proceeds to trigger normally

Prevents the review skill from posting @greptileai re-triggers before
replying to every inline Greptile comment. Adds a Step 2g verification
script that checks for unanswered comments and blocks the trigger if
any exist. Updates the Rules section to make reply-before-trigger the
explicit invariant.
@claude
Copy link
Copy Markdown

claude bot commented Mar 27, 2026

Claude finished @carlos-alm's task —— View job


Reviewing PR: fix(skill): block @greptileai trigger until all comments replied

Todo List:

  • Read and analyze the modified file
  • Phase 1: Root cause analysis
  • Phase 2: Configuration change detection
  • Phase 3: Test integrity check
  • Phase 4: Code quality review
  • Phase 5: Backlog compliance analysis
  • Provide final assessment and rating

Root Cause Analysis

Problem Being Solved: Prevents review skill from starting new review cycles while old feedback is still unanswered. Addresses workflow issue where @greptileai was triggered prematurely without replying to all existing comments.

Why This Approach: Adds mandatory verification script that exits with error code 1 if any Greptile comments lack replies. This enforces the reply-first invariant at the process level rather than relying on human discipline.

Risk Assessment: Low risk - documentation/workflow change only, no code modification. Improves review process integrity.

Backlog Compliance

  • Zero-dep: ✓ — No new dependencies, pure bash script in skill documentation
  • Foundation-aligned: ✓ — Workflow improvement, no violation of core principles
  • Problem-fit: 2 — Workflow optimization tangential to core "lost AI" problem
  • Breaking: No — Purely additive workflow guard, no API/CLI changes
  • Tier: 1

Critical Concerns

None. The verification script is well-structured with proper error handling and clear messaging.

Code Quality Assessment

  • Script logic: Sound - correctly identifies unanswered comments and blocks trigger
  • Error handling: Proper exit codes and informative error messages
  • Shell practices: Good use of arrays, proper variable handling, clear control flow
  • Integration: Fits seamlessly into existing Step 2g workflow

Final Recommendation

  • Rating: ⭐⭐⭐⭐☆ (4/5)
  • Action: APPROVE
  • Reasoning: Solid workflow improvement that addresses a real process issue. Well-implemented guard script with proper error handling. Only loses one star due to low problem-fit score (workflow optimization rather than core AI navigation problem).

The verification script at lines 208-230 effectively prevents premature @greptileai triggers and the Rules update at line 305 clearly communicates the blocking invariant. This will reduce review noise and ensure feedback is properly addressed before new review cycles begin.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 27, 2026

Greptile Summary

This PR strengthens the /review skill's Step 2g by introducing a mandatory pre-trigger verification script that blocks posting @greptileai until every root Greptile inline comment has at least one human reply. The Rules section is correspondingly tightened from "always re-trigger after replying" to "never trigger without replying to every comment first," making the reply-before-trigger order an explicit invariant.

Key changes:

  • New bash verification script added to Step 2g that fetches all PR review comments once (gh api --paginate) and checks each root Greptile comment for at least one non-Greptile reply before allowing the re-trigger to proceed.
  • All three issues flagged in prior review threads are resolved: root-only comment filtering (and .in_reply_to_id == null), N+1 API calls eliminated by reusing a single all_comments snapshot, and paginated response handling fixed with jq -s/.[][].
  • Rules section bullet rewritten to make triggering without full replies a blocking violation.
  • One residual edge case: if jq exits with an error while computing reply_count, the bash [[ -eq ]] test fails silently and the unanswered comment is not recorded, causing the verification to falsely pass.

Confidence Score: 5/5

Safe to merge — all prior thread issues are resolved and the single remaining finding is a P2 edge-case defensive-programming suggestion.

All three P1 issues from previous review threads (false-positive root-comment selection, N+1 API calls, pagination integer-comparison failure) are definitively addressed. The only remaining finding is a P2 jq-error edge case that only fires if the GitHub API returns malformed JSON, which is not a normal operating condition. Per confidence guidance, all P2 findings default to 5/5.

.claude/skills/review/SKILL.md lines 217-221 — jq error fallback in reply_count assignment.

Important Files Changed

Filename Overview
.claude/skills/review/SKILL.md Adds a mandatory pre-trigger verification script to Step 2g and hardens the Rules section; script logic is sound (root-only filter, single API fetch, -s-based pagination for reply_count), with one minor edge case where a jq error silently skips recording an unanswered comment.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Step 2g: Re-trigger Greptile] --> B["Fetch all PR review comments once\n(gh api --paginate → all_comments)"]
    B --> C["Select root Greptile comments\n(in_reply_to_id == null)"]
    C --> D{Any Greptile\nroot comments?}
    D -- No --> G[✅ Safe to re-trigger]
    D -- Yes --> E["For each comment ID:\ncount non-Greptile replies\n(jq -s .[][] | length)"]
    E --> F{reply_count\n== 0?}
    F -- Yes --> X[Add to unanswered list]
    F -- No --> E
    X --> H{unanswered\nlist empty?}
    H -- No --> I["❌ BLOCKED — print unanswered IDs\nexit 1\nGo back to Step 2e"]
    H -- Yes --> G
    G --> J{Greptile already\nreacted positively?}
    J -- Yes --> K[Skip re-trigger]
    J -- No --> L["Post @greptileai\n(re-trigger)"]
Loading

Reviews (3): Last reviewed commit: "Merge branch 'main' into fix/review-skil..." | Re-trigger Greptile

Comment on lines +210 to +211
greptile_comment_ids=$(gh api repos/optave/codegraph/pulls/<number>/comments --paginate \
--jq '[.[] | select(.user.login == "greptile-apps[bot]")] | .[].id')
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Script selects all Greptile comments, including follow-up thread replies

greptile_comment_ids is populated by selecting every comment where user.login == "greptile-apps[bot]". This includes any follow-up replies Greptile might post inside an existing thread (those will have in_reply_to_id pointing to the thread root).

When the loop later checks for human replies via select(.in_reply_to_id == $cid ...), it uses each Greptile comment's own ID as $cid. But GitHub's threading model stores in_reply_to_id as the root comment of the thread — not the ID of an intermediate reply. As a result, any Greptile follow-up reply would never appear as an in_reply_to_id for other comments, and the script would permanently flag it as unanswered, blocking the trigger even when the thread has been fully addressed.

Fix by restricting the selection to root Greptile comments only (those that are not themselves replies):

Suggested change
greptile_comment_ids=$(gh api repos/optave/codegraph/pulls/<number>/comments --paginate \
--jq '[.[] | select(.user.login == "greptile-apps[bot]")] | .[].id')
greptile_comment_ids=$(gh api repos/optave/codegraph/pulls/<number>/comments --paginate \
--jq '[.[] | select(.user.login == "greptile-apps[bot]" and .in_reply_to_id == null)] | .[].id')

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — added and .in_reply_to_id == null to the jq filter so only root Greptile comments are checked. Follow-up thread replies are now excluded, preventing false-positive blocks.

Comment on lines +214 to +219
for cid in $greptile_comment_ids; do
reply_count=$(gh api repos/optave/codegraph/pulls/<number>/comments --paginate \
--jq "[.[] | select(.in_reply_to_id == $cid and .user.login != \"greptile-apps[bot]\")] | length")
if [ "$reply_count" -eq 0 ]; then
unanswered+=("$cid")
fi
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 N+1 API calls — fetch comments once and process in-memory

Each loop iteration issues a full paginated gh api call for all PR comments just to count replies for a single $cid. For a PR with 10 Greptile comments this means 11 round-trips each retrieving the full comment list. Consider fetching all comments once and passing the payload through jq entirely:

all_comments=$(gh api repos/optave/codegraph/pulls/<number>/comments --paginate)

greptile_comment_ids=$(echo "$all_comments" \
  | jq -r '[.[] | select(.user.login == "greptile-apps[bot]" and .in_reply_to_id == null)] | .[].id')

unanswered=()
for cid in $greptile_comment_ids; do
  reply_count=$(echo "$all_comments" \
    | jq "[.[] | select(.in_reply_to_id == $cid and .user.login != \"greptile-apps[bot]\")] | length")
  if [ "$reply_count" -eq 0 ]; then
    unanswered+=("$cid")
  fi
done

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — refactored to fetch all comments once into all_comments and filter in-memory with jq. Eliminates the N+1 API call pattern.

@carlos-alm
Copy link
Copy Markdown
Contributor Author

@greptileai

Comment on lines +217 to +218
reply_count=$(echo "$all_comments" \
| jq "[.[] | select(.in_reply_to_id == $cid and .user.login != \"greptile-apps[bot]\")] | length")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 reply_count breaks on multi-page responses

gh api --paginate outputs each page as a separate JSON array, not a single merged array. When all_comments contains multiple pages (i.e., more than 30 PR review comments), the jq expression produces one number per page — e.g., 0\n2 — stored as a multi-line string in reply_count. The subsequent [ "$reply_count" -eq 0 ] bash comparison then receives a non-integer string, causing an "integer expression expected" error (exit code 2), which makes the if branch silently skip, treating every Greptile comment as answered. The check passes falsely and the block never fires.

Fix with jq -s to slurp all pages into one array before counting:

Suggested change
reply_count=$(echo "$all_comments" \
| jq "[.[] | select(.in_reply_to_id == $cid and .user.login != \"greptile-apps[bot]\")] | length")
reply_count=$(echo "$all_comments" \
| jq -s "[.[][] | select(.in_reply_to_id == $cid and .user.login != \"greptile-apps[bot]\")] | length")

The greptile_comment_ids selection above is fine as-is — jq emits one ID per line there and the for loop splits on newlines correctly. Only reply_count is affected.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — added jq -s with .[][] to slurp all paginated pages into a single array before counting replies. This prevents the multi-line integer comparison failure on PRs with >30 review comments. Also reworded the "only exception" language on line 204 so it no longer conflicts with the mandatory all-replies pre-condition.

gh api --paginate emits one JSON array per page. Without -s, jq
produces one number per page, causing the integer comparison to fail
silently and bypass the blocking check. Using jq -s slurps all pages
into a single array.

Also rewords the "only exception" language on the Greptile re-trigger
paragraph so it no longer conflicts with the mandatory all-replies
pre-condition added earlier.
@carlos-alm
Copy link
Copy Markdown
Contributor Author

@greptileai

@carlos-alm carlos-alm merged commit 2c501aa into main Mar 27, 2026
12 checks passed
@carlos-alm carlos-alm deleted the fix/review-skill-greptile-guard branch March 27, 2026 21:38
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant